-
Notifications
You must be signed in to change notification settings - Fork 16.8k
Conversation
Hi @yagonobre. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@nitisht can you check this too? |
stable/minio/README.md
Outdated
@@ -90,6 +90,10 @@ The following tables lists the configurable parameters of the Minio chart and th | |||
| `persistence.storageClass` | Type of persistent volume claim | `generic` | | |||
| `persistence.accessMode` | ReadWriteOnce or ReadOnly | `ReadWriteOnce` | | |||
| `resources` | CPU/Memory resource requests/limits | Memory: `256Mi`, CPU: `100m` | | |||
| `defaultBucket.enabled` | If true, a bucket will be create after minio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/create/created
command: ["/bin/sh", "-c", | ||
"/usr/bin/mc config host add myminio http://$MINIO_ENDPOINT:9000 $MINIO_ACCESS_KEY $MINIO_SECRET_KEY;", | ||
"/usr/bin/mc rm -r --force myminio/{{ .Values.defaultBucket.name }};", | ||
"/usr/bin/mc mb myminio//{{ .Values.defaultBucket.name }};", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
myminio/ instead of myminio//
5371b47
to
ec9031d
Compare
Thank you for the PR @yagonobre, works as expected. There is one issue I can see - after deleting a release via |
I run |
ec9031d
to
89a9a05
Compare
👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise this looks good
stable/minio/values.yaml
Outdated
@@ -54,6 +54,17 @@ resources: | |||
memory: 256Mi | |||
cpu: 250m | |||
|
|||
## Create a bucket after minio install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yagonobre Let's change this documentation to be consistent with the rest:
## Create a bucket after minio install
##
defaultBucket:
enabled: false
## If enabled, must be a string with length > 0
# name: example
## Can be one of none|download|upload|public
# policy: download
If there is a direct reference to these options, you may also want to add a ## ref: LINK
(I'm not familiar enough with Minio yet to know this info, and didn't see it in the docs after a few moments looking in https://docs.minio.io/).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that dont exist a direct link
@nitisht, have you info about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default bucket just gives an option to users who want to have a bucket created when they deploy Minio, so there is no doc link for this. I will make a note to add a link here, if we add a doc on docs.minio.io
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nitisht - OK great 👍
@yagonobre This default in the values file should still be updated to this (which adds documentation as comments, and is more DRY):
+## Create a bucket after minio install
+##
+defaultBucket:
+ enabled: false
+ ## If enabled, must be a string with length > 0
+ # name: example
+ ## Can be one of none|download|upload|public
+ # policy: download
+
Instead of what you have in the PR now (which is redundant):
+## Create a bucket after minio install
+##defaultBucket
+## enabled: true
+## name: bucketName
+## policy: "none|download|upload|public"
+##
+defaultBucket:
+ enabled: false
+ name:
+ policy: "download"
+
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated :)
stable/minio/README.md
Outdated
@@ -90,6 +90,10 @@ The following tables lists the configurable parameters of the Minio chart and th | |||
| `persistence.storageClass` | Type of persistent volume claim | `generic` | | |||
| `persistence.accessMode` | ReadWriteOnce or ReadOnly | `ReadWriteOnce` | | |||
| `resources` | CPU/Memory resource requests/limits | Memory: `256Mi`, CPU: `100m` | | |||
| `defaultBucket.enabled` | If true, a bucket will be created after minio | |||
install | `false` | | |||
| `defaultBucket.name` | Bucket name | ` ` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default should be nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
stable/minio/README.md
Outdated
| `defaultBucket.enabled` | If true, a bucket will be created after minio | ||
install | `false` | | ||
| `defaultBucket.name` | Bucket name | ` ` | | ||
| `defaultBucket.policy` | Bucket policy | `Download` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default should be download
(lowercase).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
89a9a05
to
6069a91
Compare
6069a91
to
f12ac1b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/ok-to-test |
No description provided.